You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I am unsure as to what the logic beforehand was trying to accomplish. When would api/ ever be present in the base_url? Not sure if this was a typo of some sort. I've modified the logic to just strip api from the baseurl.
This fixes the situation we are encountering with a base_url of: https://api.git.local resulting in a html url of https://github.com
The new URL parsing logic assumes that API URLs always follow the pattern of having 'api.' prefix. This may not handle all GitHub Enterprise configurations where custom API URLs are used.
base_url = urlparse(self.base_url)
-self.base_url_html = f"{base_url.scheme}://{base_url.netloc[4:] if base_url.netloc.startswith('api.') else 'https://github.com'}"+if not base_url.scheme or not base_url.netloc:+ raise ValueError("Invalid base URL format")+self.base_url_html = f"{base_url.scheme}://{base_url.netloc[4:] if base_url.netloc.startswith('api.') else 'github.com'}"
Apply this suggestion
Suggestion importance[1-10]: 8
__
Why: Adding validation for URL components is crucial for preventing runtime errors and ensuring proper URL construction. This defensive programming approach would catch malformed URLs early.
Medium
General
Preserve original URL scheme
The URL scheme should be preserved from the original base_url instead of hardcoding 'https' in the fallback case. This ensures compatibility with different URL schemes (like 'http').
-self.base_url_html = f"{base_url.scheme}://{base_url.netloc[4:] if base_url.netloc.startswith('api.') else 'https://github.com'}"+self.base_url_html = f"{base_url.scheme}://{base_url.netloc[4:] if base_url.netloc.startswith('api.') else 'github.com'}"
Apply this suggestion
Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that hardcoding 'https' in the fallback case could cause issues with different URL schemes. Preserving the original scheme improves compatibility and correctness.
Medium
More
Author self-review: I have reviewed the PR code suggestions, and addressed the relevant ones.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Type
Bug fix
I am unsure as to what the logic beforehand was trying to accomplish. When would
api/ever be present in the base_url? Not sure if this was a typo of some sort. I've modified the logic to just stripapifrom the baseurl.This fixes the situation we are encountering with a base_url of:
https://api.git.localresulting in a html url ofhttps://garden.eu.orgThis logic will handle any case:
Description
Fixed the logic for determining
base_url_html.Improved handling of
base_urlparsing usingurlparse.Changes walkthrough 📝
github_provider.py
Fix and enhance `base_url_html` logicpr_agent/git_providers/github_provider.py
base_url_htmldetermination.urlparsefor parsingbase_url.base_urlto ensure correct HTML URL generation.